Add support for object attributes in Iter call #63

fpetkovski · 2023-07-14T11:12:31Z

Add support for IterWithAttributes

This commit adds support for an IterWithAttributes on the bucket client.
The method allows iterating through objects and getting multiple attributes
into the callback function, removing the need to do an Iter followed by Attrs.

For now, we only support getting the last updated time as an attribute, but the
implementation allows adding more in the future.

Not all buckets support this method. The client can check whether the bucket has
support by calling the SupportedIterOptions method on the client.

Co-authored-by: Ashwanth Goli [email protected]
Co-authored-by: Filip Petkovski [email protected]

Signed-off-by: Filip Petkovski [email protected]

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Add support for IterWithAttributes.

Verification

Unit tests.

brancz

I like this change a lot!

brancz · 2023-07-14T11:44:51Z

prefixed_bucket.go

-	return p.bkt.Iter(ctx, pdir, func(s string) error {
-		return f(strings.TrimPrefix(s, p.prefix+DirDelim))
+	return p.bkt.Iter(ctx, pdir, func(s string, _ ObjectAttributes) error {
+		return f(strings.TrimPrefix(s, p.prefix+DirDelim), EmptyObjectAttributes)


why wouldn't it be passed through here?

brancz · 2023-07-14T11:48:29Z

objstore.go

 	// Entries are passed to function in sorted order.
-	Iter(ctx context.Context, dir string, f func(string) error, options ...IterOption) error
+	Iter(ctx context.Context, dir string, f func(name string, attrs ObjectAttributes) error, options ...IterOption) error


If we're already breaking this signature, I think we should instead make the whole thing pass only the ObjectAttributes struct and add the Name field to it.

This makes sense, and would allow us to extend supported attributes in the future.

kakkoyun

Rather than breaking the API, could we add a new API?

fpetkovski · 2023-08-09T15:20:41Z

Yeah I will look into adding a new APi. I am still not sure how to handle cases where a provider does not support returning attributes.

pracucci · 2023-08-09T15:26:59Z

Yeah I will look into adding a new APi. I am still not sure how to handle cases where a provider does not support returning attributes.

An idea may be having a function on the objstore client to check if the feature is supported. Then the Iter() contract is that if the feature is supported, attributes will be fetched, otherwise if the feature is not supported it will error out. It's caller responsability to check if the feature is supported by the actual objstore client and take an informed decision about requesting attributes or not.

ashwanthgoli · 2024-10-09T05:41:28Z

@fpetkovski are there plans to revive this pr? :)

Loki project is planning to switch to thanos clients. Having this change will reduce the fanout of listing objects along with metadata which now requires Iter call followed by fetching attributes for all objects

This commit adds support for an IterWithAttributes on the bucket client. The method allows iterating through objects and getting multiple attributes into the callback function, removing the need to do an Iter followed by Attrs. For now, we only support getting the last updated time as an attribute, but the implementation allows adding more in the future. Not all buckets support this method. The client can check whether the bucket has support by calling the SupportedIterOptions method on the client. Co-authored-by: Ashwanth Goli <[email protected]> Co-authored-by: Filip Petkovski <[email protected]> Signed-off-by: Filip Petkovski <[email protected]>

fpetkovski · 2024-10-29T16:02:59Z

@ashwanthgoli Please take a look again, your changes should be integrated now!

JoaoBraveCoding

as far as I can see lgtm

JoaoBraveCoding · 2024-10-29T16:18:06Z

objstore.go

+// WithUpdatedAt is an option that can be applied to Iter() to
+// include the last modified time in the attributes.
+// NB: Prefixes may not report last modified time.
+// This option is currently supported for the azure, aws, bos, gcs and filesystem providers.


nit: just to match the provider name

Suggested change

// This option is currently supported for the azure, aws, bos, gcs and filesystem providers.

// This option is currently supported for the azure, s3, bos, gcs and filesystem providers.

Good catch, thanks.

JoaoBraveCoding · 2024-10-29T16:21:08Z

objstore.go

+func (i *IterObjectAttributes) SetLastModified(t time.Time) {
+	i.lastModified = t
+}


Curious question: why did we make assignments to lastModified come through a setter but not name?

i wanted to make it explicit that lastModified might not always be set, made it a private field so the getter contract can return time.Time, bool

ashwanthgoli · 2024-10-30T04:48:50Z

providers/gcs/gcs.go

 		delimiter = ""
 	}

-	query := &storage.Query{
+	it := b.bkt.Objects(ctx, &storage.Query{


@fpetkovski i didn't realise this was only fetching Name attribute before.
I think I removed changes from a previous commit of yours, we need to add it back :)

Reverted the name attribute in the default case.

ashwanthgoli · 2024-10-30T05:03:27Z

objstore.go

+	const op = OpIter
+	b.metrics.ops.WithLabelValues(op).Inc()
+
+	err := b.bkt.IterWithAttributes(ctx, dir, f, options...)


updates to b.metrics.opsDuration metric is missing

Should be there now. PTAL again.

ashwanthgoli · 2024-10-30T05:06:44Z

thank you! @fpetkovski
i left a couple of comments, but the rest lgtm

Signed-off-by: Filip Petkovski <[email protected]>

JoaoBraveCoding

lgtm

Signed-off-by: Filip Petkovski <[email protected]>

fpetkovski requested review from kakkoyun, pracucci and yeya24 July 14, 2023 11:29

brancz reviewed Jul 14, 2023

View reviewed changes

kakkoyun reviewed Aug 4, 2023

View reviewed changes

fpetkovski force-pushed the iter-object-attributes branch 3 times, most recently from a85daeb to 6ede0f1 Compare October 29, 2024 15:19

fpetkovski force-pushed the iter-object-attributes branch from 6ede0f1 to 3b23d35 Compare October 29, 2024 15:36

JoaoBraveCoding approved these changes Oct 29, 2024

View reviewed changes

ashwanthgoli reviewed Oct 30, 2024

View reviewed changes

fpetkovski force-pushed the iter-object-attributes branch from 94d26ca to 3b23d35 Compare October 30, 2024 06:36

fpetkovski added 2 commits October 30, 2024 07:37

Request name for GCS

2d25e83

Signed-off-by: Filip Petkovski <[email protected]>

Add duration metric

ac8779f

Signed-off-by: Filip Petkovski <[email protected]>

JoaoBraveCoding approved these changes Oct 30, 2024

View reviewed changes

fpetkovski added 2 commits October 31, 2024 12:03

Merge branch 'main' into iter-object-attributes

27782a0

Add changelog

2c4ff97

Signed-off-by: Filip Petkovski <[email protected]>

fpetkovski merged commit b598dce into thanos-io:main Nov 5, 2024
7 checks passed

JoaoBraveCoding mentioned this pull request Nov 6, 2024

feat(thanos): make use of the new function IterWithAttributes grafana/loki#14793

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for object attributes in Iter call #63

Add support for object attributes in Iter call #63

fpetkovski commented Jul 14, 2023 •

edited

Loading

brancz left a comment

brancz Jul 14, 2023

brancz Jul 14, 2023

fpetkovski Jul 14, 2023 •

edited

Loading

brancz Jul 14, 2023

kakkoyun left a comment

fpetkovski commented Aug 9, 2023

pracucci commented Aug 9, 2023

ashwanthgoli commented Oct 9, 2024

fpetkovski commented Oct 29, 2024

JoaoBraveCoding left a comment

JoaoBraveCoding Oct 29, 2024

fpetkovski Oct 30, 2024

JoaoBraveCoding Oct 29, 2024

ashwanthgoli Oct 30, 2024

ashwanthgoli Oct 30, 2024

fpetkovski Oct 30, 2024 •

edited

Loading

ashwanthgoli Oct 30, 2024

fpetkovski Oct 30, 2024

ashwanthgoli Oct 30, 2024

ashwanthgoli commented Oct 30, 2024

JoaoBraveCoding left a comment

	// This option is currently supported for the azure, aws, bos, gcs and filesystem providers.
	// This option is currently supported for the azure, s3, bos, gcs and filesystem providers.

Add support for object attributes in Iter call #63

Add support for object attributes in Iter call #63

Conversation

fpetkovski commented Jul 14, 2023 • edited Loading

Changes

Verification

brancz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpetkovski Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakkoyun left a comment

Choose a reason for hiding this comment

fpetkovski commented Aug 9, 2023

pracucci commented Aug 9, 2023

ashwanthgoli commented Oct 9, 2024

fpetkovski commented Oct 29, 2024

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpetkovski Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwanthgoli commented Oct 30, 2024

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

fpetkovski commented Jul 14, 2023 •

edited

Loading

fpetkovski Jul 14, 2023 •

edited

Loading

fpetkovski Oct 30, 2024 •

edited

Loading